Skip to content

Move agent api classes to bootstrap package so they're not analyzed by muzzle#1208

Merged
tylerbenson merged 1 commit into
masterfrom
tyler/move-bootstrap-package
Feb 11, 2020
Merged

Move agent api classes to bootstrap package so they're not analyzed by muzzle#1208
tylerbenson merged 1 commit into
masterfrom
tyler/move-bootstrap-package

Conversation

@tylerbenson
Copy link
Copy Markdown
Contributor

@tylerbenson tylerbenson commented Feb 10, 2020

Also move jdbc classes to bootstrap to reduce size and complexity of those reference checkers.

These changes reduce the total file size of these instrumentation classes by 600k+, which should also result in decent memory savings.

This works because datadog.trace.agent.tooling.muzzle.ReferenceCreator only visits classes in the datadog.trace.instrumentation package for evaluating compatibility. Previously the "agent api" classes were in that package, so all their dependencies were added to each instrumentation class. By moving them to a different package, muzzle only checks if they exist, rather than scan all their dependencies.

@tylerbenson tylerbenson requested a review from a team as a code owner February 10, 2020 21:44
@tylerbenson tylerbenson force-pushed the tyler/move-bootstrap-package branch 2 times, most recently from 043c4b3 to b51ff76 Compare February 10, 2020 23:05
…y muzzle

Also move jdbc classes to bootstrap to reduce size and complexity of those reference checkers.

These changes reduce the total file size of these instrumentation classes by 635k, which should also result in decent memory savings.
@tylerbenson tylerbenson force-pushed the tyler/move-bootstrap-package branch from b51ff76 to 51bffa2 Compare February 10, 2020 23:09
Copy link
Copy Markdown
Contributor

@dougqh dougqh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it we somehow handle bootstrap specially. It is hard to pinpoint how exactly this makes a difference just looking at this diff, but maybe, I'm missing amongst all the changed files.

I think some additional elaboration in the description would be helpful to me and other reviewers.

@tylerbenson
Copy link
Copy Markdown
Contributor Author

@dougqh ok, I've added more explanation in the description. Does that help?

Copy link
Copy Markdown
Contributor

@randomanderson randomanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, Github doesn't have a "ignore imports" option 😑 . The only non-import changes I saw were the JDBC moves and those look okay to me.

@tylerbenson tylerbenson merged commit 829e8f6 into master Feb 11, 2020
@tylerbenson tylerbenson deleted the tyler/move-bootstrap-package branch February 11, 2020 15:36
@tylerbenson tylerbenson added this to the 0.43.0 milestone Feb 11, 2020
@dougqh
Copy link
Copy Markdown
Contributor

dougqh commented Feb 12, 2020

I think this is the right change for now, but I think we might come back and think about couple things.

First using package to determine behavior feels a bit too magical to me. I think I'd prefer that be an explicit method on the Instrumenter.

Second, now that Oracle is starting to remove methods from the JDK, we probably do need to think about muzzling the JDK itself as well. But given that we run comprehensive version checks in CI, I think the exposure is minimal and this is fine for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants